-
Notifications
You must be signed in to change notification settings - Fork 61
Convert Sytest Name/topic keys are correct to Complement
#811
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This test has been migrated to Complement, see matrix-org/complement#811
…est_name_topic_keys_are_correct
As 15s would be quite long to wait until the test failed.
This allows us to collect all incorrect data about a room and print it. A side-effect is that one doesn't need to re-run the tests to see all the broken fields. In addition, the `must.MatchGJSON` was changed to a `should`, which now re-polls `/publicRooms`. This prevents the test from exiting early upon receiving an entry that's missing some keys.
A more accurate name for it.
This is indeed much easier to follow, and less book-keeping.
Otherwise we were (correctly) seeing the warning about unexpected rooms in every time other than the first.
Co-authored-by: Eric Eastwood <[email protected]>
Co-authored-by: Eric Eastwood <[email protected]>
Better name for the variable.
This will be a no-op if the room was already private.
| return false | ||
| if len(validationErrors) > 0 { | ||
| for _, e := range validationErrors { | ||
| t.Errorf("Validation error for room %s: %s", roomID, e.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use errors.Join(...) instead of looping 🤔?
(I don't know)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I wasn't aware of this method - that's cool to learn about. There's an example in the documentation of how to use it.
I think how it'd work is something like the following:
var validationErrors error
err := doSomething()
if err !=nil {
errors.Join(validationErrors, err)
}
...a few more of those...
if validationErrors != nil {
t.Fatalf("Validation errors for room %s:\n %s", roomID, validationErrors)
}which would avoid the loop. Though I don't think it's as readable than the simple array of errors. There's also probably a loop involved in displaying validationErrors anyways?
I think I'll leave it how it is for now, but TIL about said method, and combining errors in go in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another shout: The new meta for creating an error from another is to use %w ("error wrapping), ex. fmt.Errorf("failed to generate signing key: %w", err)
See https://go.dev/doc/go1.13#error_wrapping
Calling it out because I also learned about it recently when going over a codebase with golangci-lint
This PR converts the "Name/topic keys are correct" sytest to Complement.
This was spawned from said test failing in element-hq/synapse#19071, and not wanting to debug perl.
The test was first converted to go via Claude Code, then verified manually. Finally, the test failed when modifying what Synapse returns for
/publicRooms, which is what we want to see.